Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Shadow container #638

Merged
merged 34 commits into from
Aug 3, 2023
Merged

Shadow container #638

merged 34 commits into from
Aug 3, 2023

Conversation

eriklimakc
Copy link
Contributor

GitHub Issue (If applicable): #

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@eriklimakc
Copy link
Contributor Author

The branch has been rebased from main, I'm getting some restore failures.

2
1

@Xiaoy312

@eriklimakc eriklimakc force-pushed the dev/jmaf/shadow-container branch 4 times, most recently from e703a9c to 18441fc Compare July 14, 2023 14:02
agneszitte
agneszitte reviewed Jul 18, 2023
View reviewed changes
doc/controls/ShadowContainer.md Outdated Show resolved Hide resolved
doc/controls/ShadowContainer.md Outdated Show resolved Hide resolved
doc/controls/ShadowContainer.md Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/Shadow.cs Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/Shadow.cs Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.cs Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.cs Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.cs Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jul 18, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@jeromelaban
Copy link
Member

The sample app is now available (#638 (comment)), and below is the webassembly render. Is the clipping of the shadow expected ? /cc @eriklimakc @roubachof

image

image

@eriklimakc
Copy link
Contributor Author

eriklimakc commented Jul 19, 2023 •
edited
Loading

@jeromelaban Yes, Jean-Marie has mentioned the clipping on Android and Wasm, I think @carldebilly was the one looking into it.

unoplatform/uno#12877

@eriklimakc eriklimakc mentioned this pull request Jul 19, 2023
Closed
@github-actions
Copy link

github-actions bot commented Jul 19, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@kazo0
Copy link
Contributor

kazo0 commented Jul 19, 2023

The neumorphic textbox looks a little funky when it is focused:

Dark theme:
image

Light theme:
image

Xiaoy312
Xiaoy312 reviewed Jul 19, 2023
View reviewed changes
doc/controls/ShadowContainer.md Outdated Show resolved Hide resolved
doc/controls/ShadowContainer.md Outdated Show resolved Hide resolved
doc/controls/ShadowContainer.md Outdated Show resolved Hide resolved
doc/controls/ShadowContainer.md Show resolved Hide resolved
samples/Uno.Toolkit.Samples/Uno.Toolkit.Samples.Shared/App.xaml.cs Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.Properties.cs Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.cs Outdated Show resolved Hide resolved
src/Uno.Toolkit.UI/Controls/Shadows/ShadowsCache.cs Outdated
}
}

private readonly ConcurrentDictionary<string, CacheBucket> _shadowsCache = new ConcurrentDictionary<string, CacheBucket>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i felt like when the shadows are being modified, the old cache arent being removed properly..?

src/Uno.Toolkit.UI/Controls/Shadows/ShadowsCache.cs Outdated

public void AddOrUpdate(string key, SKImage image)
{
System.Diagnostics.Debug.WriteLine($"[ShadowsCache] AddOrUpdate => key: {key}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be remove

src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.Paint.cs Outdated
if (!_shadowPropertyChanged)
{
// If a property has changed dynamically, we don't want to cache the updated shadows
Cache.AddOrUpdate(shadowsKey, surface.Snapshot());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the instance of surface.Snapshot() dont need any lifetime management?

@github-actions
Copy link

github-actions bot commented Jul 20, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@eriklimakc
Copy link
Contributor Author

About the WASM clipping, on SkiaSharp 2.88.4-preview.84 we get a better result:

image

@jeromelaban @roubachof

@github-actions
Copy link

github-actions bot commented Jul 20, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

jeromelaban
jeromelaban reviewed Jul 20, 2023
View reviewed changes
src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.Paint.cs Outdated
return needsPaint;
}

#if false // ANDROID (see comment in ShadowContainer.cs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to keep this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created this issue about that: #694

src/Uno.Toolkit.UI/Controls/Shadows/ShadowContainer.Properties.cs Outdated Show resolved Hide resolved
@eriklimakc eriklimakc mentioned this pull request Jul 25, 2023
Closed
@eriklimakc
Copy link
Contributor Author

Android and iOS control overlap issue: #679

@roubachof
Copy link
Contributor

The neumorphic textbox looks a little funky when it is focused:

Dark theme: image

Light theme: image

@kazo0 There is no implementation of neumorphism for visual states. This is just applied on the control itself.

@github-actions
Copy link

github-actions bot commented Jul 21, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@kazo0 kazo0 force-pushed the dev/jmaf/shadow-container branch from dfa25df to d8d9b93 Compare August 1, 2023 16:57
@github-actions
Copy link

github-actions bot commented Aug 1, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@github-actions
Copy link

github-actions bot commented Aug 1, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@github-actions
Copy link

github-actions bot commented Aug 2, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@github-actions
Copy link

github-actions bot commented Aug 3, 2023 •
edited by unfurl-links bot
Loading

Azure Static Web Apps: Your stage site is ready! Visit it here: https://polite-field-01aa64f0f-638.eastus2.azurestaticapps.net

@agneszitte agneszitte merged commit 97bc94f into main Aug 3, 2023
15 checks passed
@agneszitte agneszitte deleted the dev/jmaf/shadow-container branch August 3, 2023 20:00
@agneszitte
Copy link
Contributor

@eriklimakc if you can help to work on the unresolved comments we left open on another PR, please
We will sync with you tomorrow for it

@eriklimakc eriklimakc mentioned this pull request Aug 4, 2023
Merged
@agneszitte
Copy link
Contributor

agneszitte commented Aug 4, 2023 •
edited
Loading

@eriklimakc if you can help to work on the unresolved comments we left open on another PR, please We will sync with you tomorrow for it

(cc @jeromelaban)

@agneszitte
Copy link
Contributor

agneszitte commented Aug 8, 2023 •
edited by unfurl-links bot
Loading

https://github.com/Mergifyio backport legacy/3x

GitHub
Automate your pull requests. Mergify has 21 repositories available. Follow their code on GitHub.

@mergify
Copy link
Contributor

mergify bot commented Aug 8, 2023 •
edited
Loading

backport legacy/3x

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 8, 2023
@eriklimakc @mergify
Shadow container (#638)
cd8c870 
Co-authored-by: roubachof <[email protected]>
Co-authored-by: xiaoy312 <[email protected]>
Co-authored-by: Jérôme Laban <[email protected]>
Co-authored-by: Steve Bilogan <[email protected]>
(cherry picked from commit 97bc94f)

# Conflicts:
#	build/workflow/templates/dotnet-workload-install-mac.yml
#	build/workflow/templates/dotnet-workload-install-windows.yml
#	samples/Directory.Packages.props
#	samples/Uno.Toolkit.WinUI.Samples/Uno.Toolkit.WinUI.Samples.Mobile/Uno.Toolkit.WinUI.Samples.Mobile.csproj
#	src/Uno.Toolkit.sln
@mergify mergify bot mentioned this pull request Aug 8, 2023
Merged
eriklimakc added a commit that referenced this pull request Aug 15, 2023
@eriklimakc
Shadow container (#638)
5804453 
Co-authored-by: roubachof <[email protected]>
Co-authored-by: xiaoy312 <[email protected]>
Co-authored-by: Jérôme Laban <[email protected]>
Co-authored-by: Steve Bilogan <[email protected]>
(cherry picked from commit 97bc94f)
pictos pushed a commit that referenced this pull request Aug 17, 2023
@eriklimakc @pictos
Shadow container (#638)
afb5add 
Co-authored-by: roubachof <[email protected]>
Co-authored-by: xiaoy312 <[email protected]>
Co-authored-by: Jérôme Laban <[email protected]>
Co-authored-by: Steve Bilogan <[email protected]>
(cherry picked from commit 97bc94f)
pictos pushed a commit that referenced this pull request Aug 18, 2023
@eriklimakc @pictos
Shadow container (#638)
1e24735 
Co-authored-by: roubachof <[email protected]>
Co-authored-by: xiaoy312 <[email protected]>
Co-authored-by: Jérôme Laban <[email protected]>
Co-authored-by: Steve Bilogan <[email protected]>
(cherry picked from commit 97bc94f)
agneszitte pushed a commit that referenced this pull request Aug 19, 2023
@mergify @roubachof
@Xiaoy312 @jeromelaban @kazo0 @eriklimakc @pictos
Shadow container (backport #638) (#703)
c30ea9d 
Co-authored-by: roubachof <[email protected]>
Co-authored-by: xiaoy312 <[email protected]>
Co-authored-by: Jérôme Laban <[email protected]>
Co-authored-by: Steve Bilogan <[email protected]>
Co-authored-by: Érik Lima <[email protected]>
Co-authored-by: eriklimakc <[email protected]>
Co-authored-by: Pedro Jesus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roubachof roubachof roubachof approved these changes

@Xiaoy312 Xiaoy312 Xiaoy312 left review comments

@kazo0 kazo0 kazo0 left review comments

@jeromelaban jeromelaban jeromelaban left review comments

@agneszitte agneszitte agneszitte left review comments

@dr1rrb dr1rrb Awaiting requested review from dr1rrb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Specs] ShadowContainer
6 participants
@eriklimakc @jeromelaban @kazo0 @roubachof @agneszitte @Xiaoy312